#2198 preserve query strings containing slashes - #2231

Thanks @sean0407, this fixes what #2198 reports. I checked the three URLs from the issue and they now come back unchanged, including with queryElementsToRemove. The existing repairs (http://google.com&d=4&good=true → http://google.com?d=4&good=true, http://dev.com/s&utax/NEWSRLSEfy18.pdf left alone) work as before.

A few things to look at before merging.

1. A ? in the fragment still blocks the repair

The new guard ignores everything after #, so that a ? in the fragment doesn't prevent a real repair. But the rest of the method still works on the whole URL, fragment included, and the old ? check on the last element still sees it. You only see this with removeAnchorPart: false; with the default config the fragment is already gone when unmangleQueryString is called.

Splitting off the fragment once at the start, working on the part before it and putting it back at the end fixes this. It also makes the ? check on the last element unnecessary. Something along these lines:

private String unmangleQueryString(String urlToFilter) {
    // the fragment is not part of the query: leave it out of the checks
    // and put it back as is
    final int hash = urlToFilter.indexOf('#');
    final String url = hash == -1 ? urlToFilter : urlToFilter.substring(0, hash);
    if (url.indexOf('?') != -1) {
        // there is already a query
        return urlToFilter;
    }
    // only look at what follows the last slash, so that an & in an
    // earlier path segment is not mistaken for the start of a query
    final int lastSlash = url.lastIndexOf('/');
    final String last = url.substring(lastSlash + 1);
    final int firstAmp = last.indexOf('&');
    if (firstAmp == -1 || last.indexOf('=') <= 0) {
        return urlToFilter;
    }
    return url.substring(0, lastSlash + 1)
            + last.substring(0, firstAmp)
            + '?'
            + last.substring(firstAmp + 1)
            + (hash == -1 ? "" : urlToFilter.substring(hash));
}

2. Tests

The new test only uses an empty config. It would be worth adding:

• The third URL from the issue, with queryElementsToRemove: ["utm_source"], since that's where the bug did the most damage:

  input	before this PR	with this PR
  http://a.example/login/?next=/a/b/&utm_source=ref&z=1	http://a.example/login/?next=%2Fa%2Fb%2F%3Futm_source%3Dref&z=1 (utm_source stuck inside next)	http://a.example/login/?next=%2Fa%2Fb%2F&z=1

• The fragment case from point 1, with removeAnchorPart: false. At the moment no test reaches the fragment branch of the new guard.

Nits

• The Javadoc of unmangleQueryString still only describes the & → ? repair. It should say that a URL already containing a ? before the fragment is left alone, and why only the last path segment is checked (so an & in an earlier segment isn't taken for the start of a query).

@sean0407 please address comments + format code when u commit.

Thanks for catching this. I changed the split to preserve trailing empty path segments with url.split("/", -1) and added regression cases for both removeAnchorPart=false with a fragment and the default settings. The isolated Java 25 run reproduced both failures before the fix; BasicURLNormalizerTest now passes 29/29. git diff --check passed.

thanks @sean0407 , a few suggestions below

• BasicURLNormalizer.java:344-346 — the fragment and the URL are both cut out on every call, even when the URL has no &. This code runs on every outlink. Return early with if (urlToFilter.indexOf('&') == -1) return urlToFilter; before any substring.

• BasicURLNormalizer.java:349-359 — the split/String.join round trip isn't needed:

  • Use int lastSlash = url.lastIndexOf('/') to find the last segment instead of building an array.
  • firstAmp is already computed, so build the result with substring(0, firstAmp) + '?' + substring(firstAmp + 1) instead of the regex-based replaceFirst("&", "?").

• BasicURLNormalizerTest.java:216-230 — this test puts two hard-coded inputs in one method. Make it a @ParameterizedTest so a failure names the input that failed. Add the issue's third row (http://a.example/login/?next=/a/b/&utm_source=ref&z=1 with empty params, expected unchanged), which is only tested through utm_source removal right now.

• BasicURLNormalizerTest.java:243-263 — the two trailing-slash tests are the same test with a different config. Merge them into one parameterized test. The message "A trailing slash before a fragment must not be mistaken for a query" is unclear; say what is actually checked (the URL is left unchanged).

• The repair still works: the issue's http://a.example/p&a=b&c=d → http://a.example/p?a=b&c=d case has no test. The existing tests only cover the host-level case and & in an earlier segment.

• An & in the fragment is no longer rewritten: http://a.example/p#a&b=c with removeAnchorPart: false should stay unchanged. The old code gave p#a?b=c, and the new code fixes this, but no test pins it.